Skip to content

Enable resilience for _MatchingEngine and _StringProcessing. #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

rxwei
Copy link
Contributor

@rxwei rxwei commented Jan 11, 2022

_MatchingEngine and _StringProcessing are built with resilience enabled in Swift core libs. Currently the compiler build produces lots of warnings of missing @unknown default. We enable resilience in this package so that we can catch these issues early.

@rxwei rxwei requested a review from milseman January 11, 2022 03:17
@rxwei
Copy link
Contributor Author

rxwei commented Jan 11, 2022

@swift-ci please test Linux

@rxwei
Copy link
Contributor Author

rxwei commented Jan 11, 2022

:(

19:18:12 [144/179] Compiling _StringProcessing CharacterClass.swift
19:18:12 error: compile command failed due to signal 6 (use -v to see invocation)Begin Error in Function: '$s17_StringProcessing7compile_7optionsAA6RECodeV15_MatchingEngine3ASTO_AA9REOptionsVtKF0C4NodeL_yyAHKF'
19:18:12 Error! Found a leak due to a consuming post-dominance failure!
19:18:12 Value:   %90 = load [take] %89 : $*{ var AST.Atom }      // users: %286, %242, %158, %143, %123, %91
19:18:12 Post Dominating Failure Blocks:
19:18:12  bb15
19:18:12 End Error in Function: '$s17_StringProcessing7compile_7optionsAA6RECodeV15_MatchingEngine3ASTO_AA9REOptionsVtKF0C4NodeL_yyAHKF'
19:18:12 Found ownership error?!
19:18:12 triggering standard assertion failure routine
19:18:12 

@milseman
Copy link
Member

In the end, do we want to do this or would we want our sources to be slurped into the stdlib's Swift module? We'll need to figure out a direction regarding _Unicode, as we'll need to be able to call the same internal functionality as the stdlib.

@rxwei
Copy link
Contributor Author

rxwei commented Jan 11, 2022

IMO this should be part of the Swift module when it also becomes a package. Either way, the package will be built with resilience enabled in the compiler, and I think the package build config should be as close to that as possible.

@rxwei
Copy link
Contributor Author

rxwei commented Jan 12, 2022

This is failing with DEVELOPMENT-SNAPSHOT-2022-01-09-a (rdar://87427450). Note if I simply added -enable-library-evolution without fixing warnings by adding @unknown default:, the package compiles successfully. This is likely a SILGen bug, and I'm looking into it now.

Copy link
Member

@milseman milseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d slightly prefer sticking frozen on the internal module’s enums, but otherwise LGTM

@rxwei
Copy link
Contributor Author

rxwei commented Jan 12, 2022

I’d slightly prefer sticking frozen on the internal module’s enums,

I'll do that for the internal ones. CharacterClass however is the one that's triggering the SILGen bug, and it's public, so I still need to fix the bug.

Update: My bad. AST is triggering it. So marking AST as @frozen should fix it.

_MatchingEngine and _StringProcessing are built with resilience enabled in Swift core libs. Currently the compiler build produces lots of warnings of missing `@unknown default`. We enable resilience in this package so that we can catch these issues early.
@rxwei
Copy link
Contributor Author

rxwei commented Jan 12, 2022

Ok, added a bunch of @frozen and there's no need for any @unknown default: now.

@rxwei
Copy link
Contributor Author

rxwei commented Jan 12, 2022

@swift-ci please test linux

@rxwei rxwei merged commit e9ccfdc into swiftlang:main Jan 12, 2022
@rxwei rxwei deleted the resilience branch January 12, 2022 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants